-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
don't use the domain name as a filename in /ipns/a.com #5564
Conversation
fixes #5369 License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
@@ -153,6 +153,7 @@ func TestGatewayGet(t *testing.T) { | |||
ns["/ipns/double.example.com"] = path.FromString("/ipns/working.example.com") | |||
ns["/ipns/triple.example.com"] = path.FromString("/ipns/double.example.com") | |||
ns["/ipns/broken.example.com"] = path.FromString("/ipns/" + k) | |||
ns["/ipns/example.man"] = path.FromString("/ipfs/" + k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it that .man
has special meaning to the go http library? I would add a test for this. To me it is very non-obvious.
Alternatively I would have the content be a small html file and use example.txt
instead and make sure the content type is an html file not a text file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it that .man has special meaning to the go http library? I would add a test for this. To me it is very non-obvious.
Alternatively I would have the content be a small html file and use example.txtinstead and make sure the content type is an html file not a text file.
I picked .man
because it's a valid TLD. I couldn't get this to work with .html
or .txt
because IPFS will refuse to resolve these as domains. I considered using .com
but I needed something that could be a COM file (the original issue used a GIF). I didn't want to deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment.
License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
// We picked .man because: | ||
// 1. It's a valid TLD. | ||
// 2. Go treats it as the file extension for "man" files (even though | ||
// nobody actually *uses* this extension, AFAIK). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is system dependent. http.ServeContext calls mime.TypeByExtension which in the sandbox go environment does not detect it as a application/x-troff-man
. See: https://play.golang.org/p/ohNcHHsVArq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Damn, that's why .com
wasn't working.
core/corehttp/gateway_test.go
Outdated
// 1. It's a valid TLD. | ||
// 2. Go treats it as the file extension for "man" files (even though | ||
// nobody actually *uses* this extension, AFAIK). | ||
// 3. Go accepts "fnord" (the test value) as a valid man file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
Did you mean go does not accept "fnord" as a valid man file?
http.DetectContentType([]byte("fnord"))
returns text/plain; charset=utf-8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It accepts it as a valid man file if the extension is ".man". However, it looks like that part isn't important. I thought go looked at both the content and the extension but that doesn't appear to be the case (#5564 (comment)).
I don't know how I can make these tests platform-independent. At least they appear to pass on Linux now...
License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
I've merged this with a comment on the platform-dependence of the test. The won't fail on other platforms, it just won't properly test the issue. |
don't use the domain name as a filename in /ipns/a.com This commit was moved from ipfs/kubo@0fb2020
fixes #5369
This should be an uncontroversial fix while we debate content type detection in
general.